-
-
Notifications
You must be signed in to change notification settings - Fork 225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement video player in Vite branch #1878
Conversation
ThibaultNocchi
commented
Jan 31, 2023
•
edited
Loading
edited
- Remove unneeded logs
- Remove unneeded CSS styles and classes
- Skim over the code with a clear mind
2c39aaf
to
46bcf8a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from all these (quick) comments. Why you created your custom components instead of using the templates I set here and here
I don't see why OsdPlayer must be a component, I don't see them being reusable at all, given that the mini player OSD is really different to the fullscreen video player and I'd rather not have another component filled with v-if
and such.
I'm even in favor to break everything as much as possible, and make individual components for the play/pause, back, forward, repeat and shuffle buttons. Yes, they're really simple, but having them reimplemented multiple times is error and inconsistency prone.
@@ -793,7 +815,7 @@ class PlaybackManagerStore { | |||
!isNil(this.currentItem.Id) && | |||
!isNil(remote.auth.currentUser) | |||
) { | |||
await this._reportPlaybackStopped(this.currentItem.Id); | |||
this._reportPlaybackStopped(this.currentItem.Id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
await is needed, otherwise the promise won't work Why this change? The timeout already lets us be async without touching the original function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you await the reportPlayback, then when you close the player there's a non null time where nothing happens (cause it's waiting for the API call to end) before the players closes.
We could just return the promise sent by the reportPlayback then.
|
0b85f15
to
a03eb41
Compare
fd211d0
to
641d7e9
Compare
24328fa
to
d3a106f
Compare
c9b1e2e
to
18bd74b
Compare
48eb2ea
to
060f889
Compare
060f889
to
a617fc8
Compare
a617fc8
to
997a035
Compare
5e40cc7
to
462e9b4
Compare
462e9b4
to
4b2f9e9
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Cloudflare Pages deployment
|